fix(delegate): sort by fields from delegate base #294
Conversation
WalkthroughRenames a language loader parameter, centralizes the ExpressionBuilder in CRUD dialects (removing eb params), adds post-update id-consistency checks in policy handling, extends testtools loadSchema to write multiple schema modules to a temp dir, and adds many regression tests. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🧰 Additional context used📓 Path-based instructions (1){packages,samples,tests}/**📄 CodeRabbit inference engine (CLAUDE.md)
Files:
🧬 Code graph analysis (1)packages/plugins/policy/src/policy-handler.ts (3)
🔇 Additional comments (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Claude finished @ymc9's task —— View job PR Review CompleteThis PR fixes delegate sorting issues by properly handling fields from delegate base models during ordering operations. Here's my comprehensive review: 🔍 Core Changes Analysis1. Primary Fix in
|
There was a problem hiding this comment.
Pull Request Overview
This pull request implements a fix for delegate pattern sorting functionality, ensuring that sorting operations correctly reference fields from the delegate base model. The key change is in the runtime client's CRUD dialect where field references for ordering operations now properly resolve to the origin model when dealing with delegated fields.
- Modified the base CRUD dialect to properly handle field references for delegate model sorting
- Added comprehensive test coverage for various delegate pattern scenarios
- Updated utility functions to support additional schema files for testing
Reviewed Changes
Copilot reviewed 32 out of 32 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| packages/runtime/src/client/crud/dialects/base-dialect.ts | Updated field reference logic for orderBy operations to use origin model fields when available |
| packages/testtools/src/schema.ts | Enhanced loadSchema function to support additional schema files for testing |
| packages/language/src/index.ts | Renamed parameter for clarity in loadDocument function |
| packages/plugins/policy/src/policy-handler.ts | Added validation for post-update policies to prevent ID field updates |
| tests/regression/test/v2-migrated/issue-764.test.ts | Removed unnecessary $unuseAll() call |
| tests/regression/test/v2-migrated/issue-*.test.ts | Added multiple regression tests for delegate pattern scenarios |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (29)
tests/regression/test/v2-migrated/issue-1427.test.ts (1)
32-39: Add null check for better test diagnostics.If no user is found, accessing
.profileon line 39 will throw an unclear error. Consider adding an assertion thatfoundis not null before checking the profile field.const found = await db.user.findFirst({ select: { id: true, name: true, profile: false, }, }); + expect(found).toBeDefined(); expect(found.profile).toBeUndefined();tests/regression/test/v2-migrated/issue-1235.test.ts (1)
4-21: Well-structured regression test for id update validation.The test correctly validates that updating ID fields is rejected under policy enforcement. The inline schema with the
before().id != iddeny rule, combined with the id update attempt and error assertion, appropriately exercises the id-consistency check mentioned in the PR objectives.Consider renaming "regression1" to something more descriptive like "should reject id field updates with before().id != id syntax" to improve test readability. However, generic names are acceptable for regression test suites.
tests/regression/test/v2-migrated/issue-1467.test.ts (1)
7-10: Optional: Remove unused User model.The
Usermodel is defined but never used in the test. While this doesn't affect correctness, removing it would simplify the test schema.Apply this diff to remove the unused model:
- model User { - id Int @id @default(autoincrement()) - type String - } - model Container {tests/regression/test/v2-migrated/issue-1487.test.ts (1)
41-41: Consider stronger typing for test assertions.The
anytype reduces type safety. Consider using a more specific type or inline type assertions for better compile-time validation.Apply this diff to improve type safety:
- order.lineItems.forEach((item: any) => { + order.lineItems.forEach((item) => { expect(Decimal.isDecimal(item.price)).toBe(true); expect(item.price.toString()).not.toEqual('[object Object]'); }); const lineItems = await db.LineItem.findMany(); - lineItems.forEach((item: any) => { + lineItems.forEach((item) => { expect(item.createdAt instanceof Date).toBe(true); expect(Decimal.isDecimal(item.price)).toBe(true); expect(item.price.toString()).not.toEqual('[object Object]');Also applies to: 47-47
tests/regression/test/v2-migrated/issue-1483.test.ts (1)
59-66: Consider enhancing the assertion for better test coverage.The query correctly tests the delegation and relation behavior. However, the test only validates the result count. For more robust verification, consider asserting on the returned data structure.
Apply this diff to enhance the test with more thorough assertions:
- await expect( - db.edit.findMany({ + const edits = await db.edit.findMany({ include: { author: true, entity: true, }, - }), - ).resolves.toHaveLength(1); + }); + + expect(edits).toHaveLength(1); + expect(edits[0].author).toBeNull(); + expect(edits[0].entity).toMatchObject({ + id: person.id, + name: 'test', + type: 'Person', + });This verifies not just the count, but also that the delegation mechanism correctly populated the
entityrelation and that the optionalauthoris null as expected.tests/regression/test/v2-migrated/issue-1451.test.ts (1)
36-55: Consider adding explanatory comments for test assertions.When this test is enabled, consider adding brief comments to clarify:
- Why
$unuseAll()is used for data creation (lines 36-51) but not for querying (line 53)- Why
employeeReferenceis expected to be undefined (line 55) - reference the specific deny policyThis will help future maintainers understand the test's intent, especially given the complex field-level policy logic.
tests/regression/test/v2-migrated/issue-1265.test.ts (2)
4-5: Clarify skip reason and link the issue; considertest.todountil zod schemas landMake the TODO actionable and visible in test reports.
-// TODO: zod schema support -it.skip('verifies issue 1265', async () => { +// TODO(zod-schemas, #1265): enable when zod schema support is ready +it.skip('verifies issue 1265 (pending zod schema support)', async () => {
24-26: Plan for teardown when enabling the test; consider extra assertions
- When un-skipped, ensure the client/dialect is disposed to avoid resource leaks (e.g.,
await client.$disconnect?.()), especially for Postgres runs.- Optionally add:
- A default-fill case (omitting
titleshould still pass due to@default('xyz')).- A negative case (invalid shape should fail) to guard regressions.
tests/regression/test/v2-migrated/issue-1271.test.ts (1)
125-125: Remove console logging from testsConsole noise isn’t needed for this regression test.
- console.log('test3 created:', test3);tests/regression/test/v2-migrated/issue-1410.test.ts (3)
129-132: Add assertions so the test fails on regressionsCurrently results are ignored; asserting emptiness (no seeded data) ensures failures are caught.
- await db.beer.findMany({ - include: { style: true, manufacturer: true }, - where: { NOT: { gluten: true } }, - }); + await expect( + db.beer.findMany({ + include: { style: true, manufacturer: true }, + where: { NOT: { gluten: true } }, + }), + ).resolves.toEqual([]); - await db.beer.findMany({ - include: { style: true, manufacturer: true }, - where: { AND: [{ gluten: true }, { abv: { gt: 50 } }] }, - }); + await expect( + db.beer.findMany({ + include: { style: true, manufacturer: true }, + where: { AND: [{ gluten: true }, { abv: { gt: 50 } }] }, + }), + ).resolves.toEqual([]); - await db.beer.findMany({ - include: { style: true, manufacturer: true }, - where: { OR: [{ AND: [{ NOT: { gluten: true } }] }, { abv: { gt: 50 } }] }, - }); + await expect( + db.beer.findMany({ + include: { style: true, manufacturer: true }, + where: { OR: [{ AND: [{ NOT: { gluten: true } }] }, { abv: { gt: 50 } }] }, + }), + ).resolves.toEqual([]);Optionally also import expect:
-import { it } from 'vitest'; +import { it, expect } from 'vitest';Also applies to: 134-137, 139-142
139-142: Add an orderBy on a base-field to cover the delegate-sort fixExercise sorting by a base field (abv) when querying a delegated model (Beer).
Based on PR objectives
await db.beer.findMany({ include: { style: true, manufacturer: true }, where: { OR: [{ AND: [{ NOT: { gluten: true } }] }, { abv: { gt: 50 } }] }, }); + + // Sorting by a base field from the delegate base (Drink) + await expect( + db.beer.findMany({ + include: { style: true, manufacturer: true }, + orderBy: { abv: 'asc' }, + }), + ).resolves.toEqual([]);
143-143: Disconnect client to avoid leaked handles
Add client disconnect in teardown:-}); + await db.$disconnect(); +});packages/language/src/index.ts (1)
53-58: Align naming with API: rename pluginDocs to additionalDocsMinor readability nit: now that the parameter is additionalModelFiles, consider renaming pluginDocs to additionalDocs for consistency.
- // load additional model files - const pluginDocs = await Promise.all( + // load additional model files + const additionalDocs = await Promise.all( additionalModelFiles.map((file) => services.shared.workspace.LangiumDocuments.getOrCreateDocument(URI.file(path.resolve(file))), ), ); ... - await services.shared.workspace.DocumentBuilder.build([stdLib, ...pluginDocs, document, ...importedDocuments], { + await services.shared.workspace.DocumentBuilder.build([stdLib, ...additionalDocs, document, ...importedDocuments], {Additionally, consider removing non-imported additional docs from the workspace after merging to avoid memory growth over repeated calls.
Also applies to: 71-73
packages/runtime/src/client/crud/dialects/base-dialect.ts (1)
830-836: Avoid name shadowing and reuse Kysely EB where possibleThe local buildFieldRef shadows the imported buildFieldRef and spins a new expressionBuilder, which is unnecessary in orderBy callbacks. Rename and allow passing the callback eb.
- const buildFieldRef = (model: string, field: string, modelAlias: string) => { - const fieldDef = requireField(this.schema, model, field); - const eb = expressionBuilder<any, any>(); - return fieldDef.originModel - ? this.fieldRef(fieldDef.originModel, field, eb, fieldDef.originModel) - : this.fieldRef(model, field, eb, modelAlias); - }; + const resolveOrderFieldRef = ( + model: string, + field: string, + modelAlias: string, + eb?: ExpressionBuilder<any, any>, + ) => { + const fieldDef = requireField(this.schema, model, field); + const _eb = eb ?? expressionBuilder<any, any>(); + return fieldDef.originModel + ? this.fieldRef(fieldDef.originModel, field, _eb, fieldDef.originModel) + : this.fieldRef(model, field, _eb, modelAlias); + }; @@ - result = result.orderBy( - (eb) => aggregate(eb, buildFieldRef(model, k, modelAlias), field as AGGREGATE_OPERATORS), + result = result.orderBy( + (eb) => + aggregate( + eb, + resolveOrderFieldRef(model, k, modelAlias, eb), + field as AGGREGATE_OPERATORS, + ), sql.raw(this.negateSort(v, negated)), ); @@ - result = result.orderBy( - (eb) => eb.fn.count(buildFieldRef(model, k, modelAlias)), + result = result.orderBy( + (eb) => eb.fn.count(resolveOrderFieldRef(model, k, modelAlias, eb)), sql.raw(this.negateSort(v, negated)), ); @@ - const fieldRef = buildFieldRef(model, field, modelAlias); + const fieldRef = resolveOrderFieldRef(model, field, modelAlias);Also applies to: 850-854, 861-866, 875-891
packages/testtools/src/schema.ts (2)
106-115: Create parent directories when writing additional schema filesWriting nested paths will fail if directories don’t exist.
- const filePath = path.join(tempDir, name); - fs.writeFileSync(filePath, content); + const filePath = path.join(tempDir, name); + fs.mkdirSync(path.dirname(filePath), { recursive: true }); + fs.writeFileSync(filePath, content);
99-105: Optional: clean up the temp directory after loadingConsider removing tempDir after loadDocument completes to avoid accumulating temp artifacts during test runs.
tests/regression/test/v2-migrated/issue-1416.test.ts (1)
4-35: Add behavior coverage for orderBy on delegated base fieldsThis schema-only test compiles the models, which is good. Given the PR fixes “sort by fields from delegate base,” consider adding a query-layer test that orders MyPrice/MyEntity by a base delegated field to prevent regressions (e.g., orderBy: { priceType: 'asc' } resolving through @@DeleGate).
tests/regression/test/v2-migrated/issue-1415.test.ts (1)
4-21: Cover sorting over delegated fieldsNice schema regression. To align with the delegate-sort fix, add a runtime test that queries Price ordered by a field exposed via
@@delegate(priceType)to assert correct field resolution during sort.tests/regression/test/v2-migrated/issue-1381.test.ts (1)
4-55: LGTM; optional: assert policy parsing edgeSchema compiles via loadSchema. Optionally add a targeted check that the complex
@@allow("post-update", ...)expression parses/evaluates as expected under different SpaceType values.tests/regression/test/v2-migrated/issue-1014.test.ts (2)
29-30: Use standard Vitest promise matchersReplace non-standard toResolveTruthy with resolves.toBeTruthy for portability.
- await expect(db.post.update({ where: { id: post.id }, data: { authorId: user.id } })).toResolveTruthy(); + await expect(db.post.update({ where: { id: post.id }, data: { authorId: user.id } })).resolves.toBeTruthy();
44-47: Use standard Vitest promise matchersReplace non-standard toResolveNull with resolves.toBeNull; keep the second assertion as-is (already using resolves).
- await expect(db.post.findUnique({ where: { id: post.id } })).toResolveNull(); + await expect(db.post.findUnique({ where: { id: post.id } })).resolves.toBeNull();tests/regression/test/v2-migrated/issue-1243.test.ts (1)
27-28: Prefer loadSchema for compile-only checks (faster, fewer side effects)These tests don’t exercise DB ops; use loadSchema to validate schema and avoid DB setup.
- await createTestClient(schema); + await loadSchema(schema);Also applies to: 50-51
tests/regression/test/v2-migrated/issue-1466.test.ts (2)
48-51: Teardown: disconnect clients to avoid leaked connectionsCall $disconnect at test end, especially with usePrismaPush/Postgres pools.
expect(userWithAsset).toMatchObject({ asset: { assetType: 'VideoLongLongLongLongLongLongLongLongName', duration: 100 }, }); + await db.$disconnect();expect(userWithAsset).toMatchObject({ asset: { assetType: 'VideoLongLongLongLongName', duration: 100 }, }); + await db.$disconnect();Also applies to: 98-101
4-101: Add a regression for sorting by base fields via delegatesPR goal mentions sorting by fields from delegate base. Add a test that orders a subclass by a base field to guard this fix.
Example to add:
it('orders subclass by base field (delegate base)', async () => { const db = await createTestClient(` model Asset { id Int @id @default(autoincrement()) createdAt DateTime @default(now()) assetType String @@delegate(assetType) } model Video extends Asset { duration Int } `, { usePrismaPush: true }); // create two videos with different createdAt const v1 = await db.video.create({ data: { duration: 1 } }); // ensure later timestamp await new Promise(r => setTimeout(r, 10)); const v2 = await db.video.create({ data: { duration: 2 } }); const asc = await db.video.findMany({ orderBy: { createdAt: 'asc' }, select: { id: true } }); const desc = await db.video.findMany({ orderBy: { createdAt: 'desc' }, select: { id: true } }); expect(asc[0].id).toBe(v1.id); expect(desc[0].id).toBe(v2.id); await db.$disconnect(); });tests/regression/test/v2-migrated/issue-1135.test.ts (2)
38-47: Avoid a floating Promise in extraSourceFiles main.tsIf lint rules like no-floating-promises are enabled during type-check, this can flag. Explicitly ignore with void.
-db.person.create({ +void db.person.create({ data: { name: 'test', attachments: { create: { url: 'https://...', }, }, }, });
53-76: Strengthen assertion: verify FK equals parent idCapture the created entity and assert attachments[0].myEntityId === id for tighter correctness.
- await expect( - db.person.create({ - data: { - name: 'test', - attachments: { - create: { - url: 'https://...', - }, - }, - }, - include: { attachments: true }, - }), - ).resolves.toMatchObject({ - id: expect.any(String), - name: 'test', - attachments: [ - { - id: expect.any(String), - url: 'https://...', - myEntityId: expect.any(String), - }, - ], - }); + const created = await db.person.create({ + data: { + name: 'test', + attachments: { + create: { + url: 'https://...', + }, + }, + }, + include: { attachments: true }, + }); + expect(created).toMatchObject({ + id: expect.any(String), + name: 'test', + attachments: [ + { + id: expect.any(String), + url: 'https://...', + myEntityId: expect.any(String), + }, + ], + }); + expect(created.attachments[0].myEntityId).toBe(created.id);tests/regression/test/v2-migrated/issue-1080.test.ts (2)
8-8: Remove redundant @unique on @id fields@id implies uniqueness; the extra @unique is redundant noise.
- id String @id @unique @default(uuid()) + id String @id @default(uuid())Also applies to: 15-15
122-128: Assert exact collection size for stronger guaranteesAlso assert the final number of related Fields to catch accidental extras.
expect(updated).toMatchObject({ Fields: expect.arrayContaining([ expect.objectContaining({ name: 'first3' }), expect.objectContaining({ name: 'second4' }), expect.objectContaining({ name: 'third1' }), ]), }); + expect(updated.Fields).toHaveLength(3);Optionally apply similar length checks after earlier updates (Lines 54-59 and 74-79) to assert exactly two items.
tests/regression/test/v2-migrated/issue-1078.test.ts (1)
20-33: Clarify comment and add negative-case validation
- Reword the
//! This query fails validationcomment to indicate the update should succeed (name unchanged).- Add a negative-case update (e.g.
value: -1) and expect.rejects.toThrow().- No changes needed for matchers:
toResolveTruthy/toResolveNullare globally registered viaexpect.extend.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (32)
packages/language/src/index.ts(2 hunks)packages/plugins/policy/src/policy-handler.ts(1 hunks)packages/runtime/src/client/crud/dialects/base-dialect.ts(4 hunks)packages/testtools/src/schema.ts(1 hunks)tests/regression/test/v2-migrated/issue-1014.test.ts(1 hunks)tests/regression/test/v2-migrated/issue-1058.test.ts(1 hunks)tests/regression/test/v2-migrated/issue-1078.test.ts(1 hunks)tests/regression/test/v2-migrated/issue-1080.test.ts(1 hunks)tests/regression/test/v2-migrated/issue-1123.test.ts(1 hunks)tests/regression/test/v2-migrated/issue-1135.test.ts(1 hunks)tests/regression/test/v2-migrated/issue-1149.test.ts(1 hunks)tests/regression/test/v2-migrated/issue-1167.test.ts(1 hunks)tests/regression/test/v2-migrated/issue-1179.test.ts(1 hunks)tests/regression/test/v2-migrated/issue-1235.test.ts(1 hunks)tests/regression/test/v2-migrated/issue-1241.test.ts(1 hunks)tests/regression/test/v2-migrated/issue-1243.test.ts(1 hunks)tests/regression/test/v2-migrated/issue-1257.test.ts(1 hunks)tests/regression/test/v2-migrated/issue-1265.test.ts(1 hunks)tests/regression/test/v2-migrated/issue-1271.test.ts(1 hunks)tests/regression/test/v2-migrated/issue-1381.test.ts(1 hunks)tests/regression/test/v2-migrated/issue-1388.test.ts(1 hunks)tests/regression/test/v2-migrated/issue-1410.test.ts(1 hunks)tests/regression/test/v2-migrated/issue-1415.test.ts(1 hunks)tests/regression/test/v2-migrated/issue-1416.test.ts(1 hunks)tests/regression/test/v2-migrated/issue-1427.test.ts(1 hunks)tests/regression/test/v2-migrated/issue-1451.test.ts(1 hunks)tests/regression/test/v2-migrated/issue-1454.test.ts(1 hunks)tests/regression/test/v2-migrated/issue-1466.test.ts(1 hunks)tests/regression/test/v2-migrated/issue-1467.test.ts(1 hunks)tests/regression/test/v2-migrated/issue-1483.test.ts(1 hunks)tests/regression/test/v2-migrated/issue-1487.test.ts(1 hunks)tests/regression/test/v2-migrated/issue-764.test.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
{packages,samples,tests}/**
📄 CodeRabbit inference engine (CLAUDE.md)
Place packages only under
packages/,samples/, ortests/
Files:
tests/regression/test/v2-migrated/issue-1123.test.tspackages/plugins/policy/src/policy-handler.tstests/regression/test/v2-migrated/issue-1179.test.tstests/regression/test/v2-migrated/issue-1410.test.tstests/regression/test/v2-migrated/issue-1078.test.tspackages/runtime/src/client/crud/dialects/base-dialect.tstests/regression/test/v2-migrated/issue-1235.test.tstests/regression/test/v2-migrated/issue-1014.test.tspackages/testtools/src/schema.tstests/regression/test/v2-migrated/issue-1466.test.tstests/regression/test/v2-migrated/issue-1149.test.tstests/regression/test/v2-migrated/issue-764.test.tstests/regression/test/v2-migrated/issue-1058.test.tstests/regression/test/v2-migrated/issue-1080.test.tstests/regression/test/v2-migrated/issue-1167.test.tspackages/language/src/index.tstests/regression/test/v2-migrated/issue-1381.test.tstests/regression/test/v2-migrated/issue-1451.test.tstests/regression/test/v2-migrated/issue-1467.test.tstests/regression/test/v2-migrated/issue-1483.test.tstests/regression/test/v2-migrated/issue-1388.test.tstests/regression/test/v2-migrated/issue-1487.test.tstests/regression/test/v2-migrated/issue-1415.test.tstests/regression/test/v2-migrated/issue-1135.test.tstests/regression/test/v2-migrated/issue-1416.test.tstests/regression/test/v2-migrated/issue-1257.test.tstests/regression/test/v2-migrated/issue-1241.test.tstests/regression/test/v2-migrated/issue-1265.test.tstests/regression/test/v2-migrated/issue-1454.test.tstests/regression/test/v2-migrated/issue-1271.test.tstests/regression/test/v2-migrated/issue-1243.test.tstests/regression/test/v2-migrated/issue-1427.test.ts
**/schema.ts
📄 CodeRabbit inference engine (CLAUDE.md)
The generated TypeScript schema should be named
schema.ts
Files:
packages/testtools/src/schema.ts
🧬 Code graph analysis (30)
tests/regression/test/v2-migrated/issue-1123.test.ts (1)
packages/testtools/src/client.ts (1)
createPolicyTestClient(176-187)
packages/plugins/policy/src/policy-handler.ts (2)
packages/common-helpers/src/tiny-invariant.ts (1)
invariant(4-14)packages/runtime/src/client/errors.ts (1)
QueryError(18-22)
tests/regression/test/v2-migrated/issue-1179.test.ts (1)
packages/testtools/src/schema.ts (1)
loadSchema(94-124)
tests/regression/test/v2-migrated/issue-1410.test.ts (1)
packages/testtools/src/client.ts (1)
createTestClient(51-166)
tests/regression/test/v2-migrated/issue-1078.test.ts (1)
packages/testtools/src/client.ts (1)
createPolicyTestClient(176-187)
packages/runtime/src/client/crud/dialects/base-dialect.ts (1)
packages/runtime/src/client/query-utils.ts (3)
buildFieldRef(183-209)requireField(47-65)aggregate(391-399)
tests/regression/test/v2-migrated/issue-1235.test.ts (1)
packages/testtools/src/client.ts (2)
createPolicyTestClient(176-187)testLogger(189-191)
tests/regression/test/v2-migrated/issue-1014.test.ts (1)
packages/testtools/src/client.ts (1)
createPolicyTestClient(176-187)
packages/testtools/src/schema.ts (1)
packages/language/test/utils.ts (1)
loadSchema(8-19)
tests/regression/test/v2-migrated/issue-1466.test.ts (2)
packages/testtools/src/client.ts (1)
createTestClient(51-166)packages/testtools/src/schema.ts (1)
loadSchema(94-124)
tests/regression/test/v2-migrated/issue-1149.test.ts (1)
packages/testtools/src/client.ts (1)
createTestClient(51-166)
tests/regression/test/v2-migrated/issue-1058.test.ts (1)
packages/testtools/src/client.ts (1)
createTestClient(51-166)
tests/regression/test/v2-migrated/issue-1080.test.ts (1)
packages/testtools/src/client.ts (1)
createPolicyTestClient(176-187)
tests/regression/test/v2-migrated/issue-1167.test.ts (1)
packages/testtools/src/schema.ts (1)
loadSchema(94-124)
tests/regression/test/v2-migrated/issue-1381.test.ts (1)
packages/testtools/src/schema.ts (1)
loadSchema(94-124)
tests/regression/test/v2-migrated/issue-1451.test.ts (1)
packages/testtools/src/client.ts (1)
createTestClient(51-166)
tests/regression/test/v2-migrated/issue-1467.test.ts (1)
packages/testtools/src/client.ts (1)
createTestClient(51-166)
tests/regression/test/v2-migrated/issue-1483.test.ts (1)
packages/testtools/src/client.ts (1)
createTestClient(51-166)
tests/regression/test/v2-migrated/issue-1388.test.ts (1)
packages/testtools/src/schema.ts (1)
loadSchema(94-124)
tests/regression/test/v2-migrated/issue-1487.test.ts (1)
packages/testtools/src/client.ts (1)
createTestClient(51-166)
tests/regression/test/v2-migrated/issue-1415.test.ts (1)
packages/testtools/src/schema.ts (1)
loadSchema(94-124)
tests/regression/test/v2-migrated/issue-1135.test.ts (1)
packages/testtools/src/client.ts (1)
createTestClient(51-166)
tests/regression/test/v2-migrated/issue-1416.test.ts (1)
packages/testtools/src/schema.ts (1)
loadSchema(94-124)
tests/regression/test/v2-migrated/issue-1257.test.ts (1)
packages/testtools/src/schema.ts (1)
loadSchema(94-124)
tests/regression/test/v2-migrated/issue-1241.test.ts (1)
packages/testtools/src/client.ts (1)
createPolicyTestClient(176-187)
tests/regression/test/v2-migrated/issue-1265.test.ts (1)
packages/testtools/src/client.ts (1)
createTestClient(51-166)
tests/regression/test/v2-migrated/issue-1454.test.ts (1)
packages/testtools/src/client.ts (1)
createPolicyTestClient(176-187)
tests/regression/test/v2-migrated/issue-1271.test.ts (1)
packages/testtools/src/client.ts (1)
createPolicyTestClient(176-187)
tests/regression/test/v2-migrated/issue-1243.test.ts (1)
packages/testtools/src/client.ts (1)
createTestClient(51-166)
tests/regression/test/v2-migrated/issue-1427.test.ts (1)
packages/testtools/src/client.ts (1)
createTestClient(51-166)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: build-test (20.x, sqlite)
- GitHub Check: build-test (20.x, postgresql)
- GitHub Check: claude-review
🔇 Additional comments (34)
tests/regression/test/v2-migrated/issue-764.test.ts (1)
27-29: LGTM! Simplified user creation.Removing
$unuseAll()is appropriate since the User model has@@allow('all', true), making the policy bypass unnecessary. This simplifies the test while maintaining the same behavior.tests/regression/test/v2-migrated/issue-1427.test.ts (3)
1-2: Imports look good.Standard imports for testtools client creation and Vitest test utilities are appropriate.
4-21: Schema definition is valid.The one-to-one User-Profile relationship is correctly defined with proper relation decorators, foreign key, and unique constraint. The permissive access control (
@@allow('all', true)) is appropriate for a regression test.
23-30: Verify if$unuseAll()is necessary.The test bypasses all plugins/middleware before creating the user. Confirm this is intentional for isolating the test scenario for issue 1427.
tests/regression/test/v2-migrated/issue-1235.test.ts (2)
1-2: LGTM!The imports are correctly structured and all imported utilities are used in the test cases.
23-38: LGTM! Tests variation in deny rule syntax.This test case appropriately validates the same id update rejection behavior using the
this.idsyntax variant in the deny rule. Testing bothidandthis.idsyntax ensures the policy engine correctly handles different reference styles when evaluating id-consistency checks.tests/regression/test/v2-migrated/issue-1058.test.ts (1)
1-3: LGTM!The imports are correct and appropriate for a vitest-based regression test.
tests/regression/test/v2-migrated/issue-1467.test.ts (3)
1-2: LGTM!Standard test imports are correctly used.
32-37: LGTM!The test data setup is clean and appropriate. The explicit
id: 1is fine for an isolated test environment, and the two containers correctly connect to the same drink to verify the relation count behavior.
39-43: LGTM! Test correctly verifies the delegate sorting fix.The query appropriately tests sorting by a field (
name) from the delegate base model (Drink), which aligns with the PR objective to fix sorting by fields from delegate base. The_countassertion verifies that relations work correctly with delegated models.tests/regression/test/v2-migrated/issue-1454.test.ts (4)
1-2: LGTM!Imports are appropriate for policy-enforced regression testing.
5-35: LGTM!The test correctly validates that access policies on related models are enforced during filter evaluation. Since no authenticated user is present and
Userhas a restrictive read policy (auth() == this), filteringPurchasebyUserfields returns zero results as expected. Testing both implicit and explicit join syntax provides good coverage.
37-68: Clear TODO for field-level policy support.The skipped test is well-structured and ready to be enabled once field-level policies are implemented. The TODO comment clearly documents the blocker.
70-115: Comprehensive test coverage for field-level policies.The skipped test provides thorough coverage of mixed object-level and field-level policy scenarios, including OR conditions. Well-prepared for future implementation.
tests/regression/test/v2-migrated/issue-1487.test.ts (1)
1-52: LGTM! Test structure and validation logic are solid.The test correctly validates Decimal and DateTime field handling in delegated models. The schema structure with
@@delegate(entityType)andextends BaseTypeis appropriate, and the type assertions comprehensively verify that Decimal fields remain as Decimal instances and DateTime fields as Date instances after database round-trips.tests/regression/test/v2-migrated/issue-1483.test.ts (4)
1-2: LGTM!The imports are correct and appropriate for a ZenStack regression test.
4-41: Well-structured delegation test schema.The schema correctly demonstrates the delegation pattern with
Entityas the base model,Personextending it, andEdithaving a relation to the baseEntitytype. The relation configurations and access policies are appropriate.However, given the PR title "fix(delegate): sort by fields from delegate base," consider whether this test should include an
orderByclause to verify sorting behavior on delegated fields.Would you like me to suggest an enhanced test case that includes sorting by fields from the delegate base (e.g., ordering edits by
entity.name)?
43-45: LGTM!The cleanup order is correct, deleting
Editfirst to satisfy foreign key constraints, followed byPerson(which is a delegatedEntity), and finallyUser.
47-57: LGTM!The test data setup is correct. The
Personis created properly (with thetypefield automatically handled by the delegation mechanism), and theEditis correctly linked to thePersonviaentityId. The optionalauthorrelation is intentionally left null, which is valid per the schema.tests/regression/test/v2-migrated/issue-1451.test.ts (2)
24-25: Verify the deny policy logic foremployeeReference.The deny policy on line 25 uses a complex condition:
@deny("read, update", space.memberships?[auth() == user && !(role in ['owner', 'admin'])])This appears to deny read/update access when there exists a membership in the space where the authenticated user matches the membership's user AND the role is not 'owner' or 'admin'.
In the test scenario:
- User '1' is authenticated (if context is set)
- The membership has
role: 'foo'(line 48)- This should trigger the deny rule, making
employeeReferenceundefinedEnsure this policy logic correctly captures the intended access control semantics when field-level policy support is implemented.
4-5: Clarify authentication context before enabling this test.
- This test’s deny policies call
auth()but no auth middleware is applied—no other regression tests usewithAuth. Before un-skipping, either:
- Add
db.$use(withAuth({ user: { id: '1' } }))(or equivalent) before the query, or- Explicitly assert the expected behavior when
auth()is undefined.tests/regression/test/v2-migrated/issue-1265.test.ts (1)
6-7: EnsurecreateTestClientreturnszodSchemasbefore un-skipping
Tests destructurezodSchemas, but it isn’t currently exposed bycreateTestClient; add it to the returned client to avoidundefinedat runtime.tests/regression/test/v2-migrated/issue-1271.test.ts (4)
5-43: Test setup and policy schema look goodInline schema and policy-enabled client creation are appropriate for this regression scenario.
56-115: Upsert with nested relation creation is correctCovers both update-on-existing and create-on-missing paths with composite unique where; assertions are sound.
118-151: connectOrCreate (connect branch) covered wellUpdate targets composite PK, connects to existing Test via composite unique; include/assertions are precise.
157-187: connectOrCreate (create branch) covered wellExpects creation when unique target is absent; assertions verify the outcome.
tests/regression/test/v2-migrated/issue-1410.test.ts (1)
4-126: Good addition: exercises delegate base-field filtersCovers NOT/AND/OR over base fields on a delegated model with includes. Solid repro shape for the regression.
tests/regression/test/v2-migrated/issue-1257.test.ts (1)
4-48: LGTM: multi-file schema loading regression covers the intended caseThe test correctly exercises loadSchema with additional modules; failure will surface from loadSchema’s internal expect().
tests/regression/test/v2-migrated/issue-1241.test.ts (1)
68-84: Confirm matcher availability (toResolveTruthy)Does your test setup register toResolveTruthy? If not, prefer the Vitest-native form:
await expect( db.todo.update({ /* ... */ }) ).resolves.toBeTruthy();tests/regression/test/v2-migrated/issue-1179.test.ts (1)
4-25: LGTM: schema-load regression capturedThe minimal compile check for composition via
type Baseandmodel ... with Baseis appropriate here.tests/regression/test/v2-migrated/issue-1167.test.ts (1)
4-19: LGTM: minimal schema regressionCaptures the delegated-type mapping scenario; compile-only is fine for this case.
tests/regression/test/v2-migrated/issue-1388.test.ts (1)
5-31: LGTM: good use of loadSchema with module importsImports './auth' and './post' align with additionalSchemas writing to the same temp dir. No issues spotted.
tests/regression/test/v2-migrated/issue-1123.test.ts (1)
33-42: LGTM: _count assertion under policy passesSetup and assertions look correct and deterministic.
tests/regression/test/v2-migrated/issue-1149.test.ts (1)
56-82: LGTM: createMany coverage is solidSingle and batch createMany verified with count; final check uses arrayContaining to avoid order dependence.
|
Claude finished @ymc9's task —— View job PR Review for "fix(delegate): sort by fields from delegate base"This PR implements a crucial fix for delegate pattern sorting functionality in ZenStack v3. Here's my comprehensive review: 🎯 Primary Fix AnalysisCore Issue: The original code in Solution: Added a const buildFieldRef = (model: string, field: string, modelAlias: string) => {
const fieldDef = requireField(this.schema, model, field);
return fieldDef.originModel
? this.fieldRef(fieldDef.originModel, field, fieldDef.originModel)
: this.fieldRef(model, field, modelAlias);
};This ensures that when sorting by inherited fields from delegate base models, the correct model alias is used in generated SQL. ✅ Strengths
|
|
Claude finished @ymc9's task —— View job PR Review for "fix(delegate): sort by fields from delegate base"Tasks:
🎯 Executive SummaryThis PR implements a critical fix for delegate pattern sorting functionality in ZenStack v3. The primary change adds proper field reference resolution for delegate base models in the 🔍 Core Changes Analysis1. Primary Fix in
|
|
Claude finished @ymc9's task —— View job PR Review for "fix(delegate): sort by fields from delegate base"This PR implements a critical fix for delegate pattern sorting functionality in ZenStack v3. Here's my comprehensive review: Tasks:
🎯 Executive SummaryThis PR implements a critical fix for delegate pattern sorting functionality in ZenStack v3. The primary change adds proper field reference resolution for delegate base models in the 🔍 Core Changes Analysis1. Primary Fix in
|
|
Claude finished @ymc9's task —— View job PR Review for "fix(delegate): sort by fields from delegate base"I'll conduct a comprehensive review of this PR's code quality, potential issues, and test coverage. Tasks:
🎯 Executive SummaryThis PR implements a critical fix for delegate pattern sorting functionality in ZenStack v3. The primary change adds proper field reference resolution for delegate base models in the 🔍 Core Changes Analysis1. Primary Fix in
|
Summary by CodeRabbit